-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Apply flexible types to files compiled without explicit nulls #23386
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Test performance please |
a04a482
to
77c3c0c
Compare
[test_scala2_library_tasty] |
test performance please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a commit that doesn't change anything real to force the CI to re-run. (Perhaps change the newly-introduced double newline in TreeUnpickler.)
Otherwise LGTM, but also wait for approval from @noti0na1 .
77c3c0c
to
a584b72
Compare
a584b72
to
d9f49d1
Compare
@@ -3424,6 +3428,15 @@ object Types extends TypeUtils { | |||
// rule. | |||
FlexibleType(OrNull(tp), tp) | |||
} | |||
|
|||
def make(tp: Type)(using Context): Type = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need a separate constructor for FlexibleType? I feel we can just merge this logic into apply
.
def apply(tp: Type)(using Context): FlexibleType = tp match { | ||
def apply(tp: Type)(using Context): FlexibleType = | ||
assert(tp.isValueType, s"Should not flexify ${tp}") | ||
tp match { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're now nullifying more complex types in Scala, do you think it's worth bringing the commented-out logic back?
if outermostLevelAlreadyNullable then false | ||
else tp match | ||
case tp: TypeRef | ||
if !ctx.flexibleTypes && tp.isRef(defn.RepeatedParamClass) => false | ||
case _ => true | ||
|
||
override def apply(tp: Type): Type = tp match { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to consider other cases now: ExprType
, AnnotatedType
, OrType
, and MatchType
(mayber? not sure).
@@ -0,0 +1,21 @@ | |||
package unsafeNulls | |||
|
|||
class Unsafe_1 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to test more scala tests.
A => B
,=> T
A { val b: B }
,A @B
a.type
(A, B)
A | B
,A & B
- ...
Also, let's test not only the functions, but also constructors, members (value and type), extension methods, etc.
@@ -0,0 +1,18 @@ | |||
byname-nullables.scala # identity() flexified |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you choose some tests and explain them in the PR why they are excluded?
A fixed version of #22473
This PR wraps the types of symbols from files compiled without explicit nulls in flexible types.
This allows for interop between multiple files in cases where
compiled without explicit nulls can still be used in
whereas the argument would have been a strictly non-null String, because of the flexible type, the function call is now permitted.
[test_scala2_library_tasty]